Skip to content

detect and negotiate mtu#3304

Open
randomkang wants to merge 7 commits into
apache:masterfrom
randomkang:dynamic_mtu
Open

detect and negotiate mtu#3304
randomkang wants to merge 7 commits into
apache:masterfrom
randomkang:dynamic_mtu

Conversation

@randomkang
Copy link
Copy Markdown
Contributor

@randomkang randomkang commented May 18, 2026

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

What is changed and the side effects?

Changed:

Side effects:

  • Performance effects:

The bandwith of rdma perf will be increased 25%(in my case, from 33GB/s to 41GB/s ).

  • Breaking backward compatibility:

Check List:

@chenBright chenBright requested a review from Copilot May 19, 2026 04:52
Copilot AI reviewed May 19, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@chenBright chenBright requested a review from Copilot May 19, 2026 05:18
Copilot AI reviewed May 19, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

chenBright
chenBright reviewed May 19, 2026
View reviewed changes
Comment thread src/brpc/rdma/rdma_endpoint.cpp Outdated
Comment thread src/brpc/rdma/rdma_helper.cpp Outdated
@chenBright chenBright requested a review from Copilot May 20, 2026 02:20
Copilot AI reviewed May 20, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

Comment thread src/brpc/rdma/rdma_helper.cpp Outdated
Comment on lines +462 to +464

if (ibv_query_port(ctx, port_num, &port_attr)) {
LOG(ERROR) << "ibv_query_port failed";
Comment thread src/brpc/rdma/rdma_endpoint.cpp
Comment on lines +93 to 94
static uint16_t g_rdma_hello_msg_len = 42; // In Byte
static uint16_t g_rdma_hello_version = 2;
Comment thread src/brpc/rdma/rdma_endpoint.cpp
Comment on lines +1259 to +1267
LOG(INFO) << "negotiated mtu is 256";
} else if (mtu_type == IBV_MTU_512) {
LOG(INFO) << "negotiated mtu is 512";
} else if (mtu_type == IBV_MTU_1024) {
LOG(INFO) << "negotiated mtu is 1024";
} else if (mtu_type == IBV_MTU_2048) {
LOG(INFO) << "negotiated mtu is 2048";
} else if (mtu_type == IBV_MTU_4096) {
LOG(INFO) << "negotiated mtu is 4096";
Comment thread src/brpc/rdma/rdma_endpoint.cpp
Comment on lines +548 to 551
// use the minimum of local mtu type and remote mtu type
uint16_t min_mtu_type = std::min(local_mtu_type, remote_msg.mtu_type);
if (ep->BringUpQp(remote_msg.lid, remote_msg.gid, remote_msg.qp_num, min_mtu_type) < 0) {
LOG(WARNING) << "Fail to bringup QP, fallback to tcp:" << s->description();
Comment thread src/brpc/rdma/rdma_endpoint.cpp Outdated
Comment on lines 697 to 703
// Only happens in UT
local_msg.qp_num = 0;
}
local_msg.mtu_type = local_mtu_type;
}
memcpy(data, MAGIC_STR, 4);
local_msg.Serialize((char*)data + 4);
chenBright
chenBright reviewed May 20, 2026
View reviewed changes
Comment thread src/brpc/rdma/rdma_endpoint.cpp Outdated
@chenBright
Copy link
Copy Markdown
Contributor

chenBright commented May 20, 2026

Please update the RDMA UTs in brpc_rdma_unittest.cpp.

chenBright
chenBright reviewed May 21, 2026
View reviewed changes
Comment thread src/brpc/rdma/rdma_endpoint.cpp
Comment on lines +566 to +569
if (remote_msg_ext_len >= 2) {
// mtu_type field is valid
remote_mtu_type = remote_msg.mtu_type;
}
Copy link
Copy Markdown
Contributor

@chenBright chenBright May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By setting the default value of HelloMessage::mtu_type to IBV_MTU_1024, it's no longer necessary to check remote_msg_ext_len.

Copy link
Copy Markdown
Contributor Author

@randomkang randomkang May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this check, because the brpc version of peer can be older or newer, we should make sure the mtu_type field is valid in remote_msg.

@randomkang
Copy link
Copy Markdown
Contributor Author

randomkang commented May 21, 2026

Please update the RDMA UTs in brpc_rdma_unittest.cpp.

I can't find a way to check the rdma communication between the current version including mtu negotiation and without mtu negotiation in ut, any advice?

@chenBright
Copy link
Copy Markdown
Contributor

chenBright commented May 25, 2026

Please update the RDMA UTs in brpc_rdma_unittest.cpp.

I can't find a way to check the rdma communication between the current version including mtu negotiation and without mtu negotiation in ut, any advice?

You can refer to client_hello_msg_invalid_sq_rq_block_size, server_hello_invalid_sq_rq_size, etc.

yanglimingcn
yanglimingcn reviewed May 25, 2026
View reviewed changes
Comment thread src/brpc/rdma/rdma_endpoint.cpp
// Just for future use, should not happen now

// In older versions of brpc, IBV_MTU_1024 is the default mtu type,
// So we set remote_mtu IBV_MTU_1024 at default to be ompatible with older versions.
Copy link
Copy Markdown
Contributor

@yanglimingcn yanglimingcn May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compatible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yanglimingcn yanglimingcn yanglimingcn left review comments

@chenBright chenBright chenBright left review comments

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@randomkang @chenBright @yanglimingcn